-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add showOnFocus prop to EuiScreenReader Only & Add EuiSkipLink #2976
Conversation
9011e7e
to
7d8929f
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally this all works great, just some tweaks to implementation.
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
5f4d9d3
to
9e00477
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
@ryankeairns I pushed the prop change we talked about and the type, test and example updates as well. I think all it needs now are some snippets? |
Thanks @cchaos , I'll add some snippets. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
For those reviewing, the If you reviewed this previously, I was hacking around this by using a |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code LGTM! One thing I hadn't tested before though is that when the fixed version (even though it's just docs) is in a portal, since portals are at the end of the DOM, the next tab is to the browser's nav instead of the next element in the DOM structure. What was the reasoning for the portal on that one?
@cchaos placing it in a portal with a tabindex of 1 originally allowed me to fake the correct order and visual/fixed placement on the docs site. With your last commit, that index value is now forced to 0 for the I should be able to remove the portal now and it will remain the next in tab order after the switch. I’ll try that in the morning, and then we’ll just live with it until the static site structure is in place, at which point we can place it correctly atop the body and do away with the switch altogether (i.e. we can then just say “Try it on this site”). |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2976/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! 🎉
Fixes #2356
Fixes #2962
Summary
In order to address WCAG A guidelines, a skip link should be provided that allows keyboard and screen reader users to bypass navigation and jump to the main content of the page.
As discussed in #2356 , the consensus approach was to:
showOnFocus
prop toEuiScreenReaderOnly
that forces the display of visually hidden content when it receives focus via keyboard, andEuiSkipLink
component that leverages the aforementioned prop while providing additional styles.Notes
Docs fix
This PR also fixes the empty Props tab for
EuiScreenReaderOnly
as noted in #2962 . This component only returns a cloned element (no JSX), so a workaround pattern suggested by @chandlerprall was used to populate the Props tab.Regarding the
EuiSkipLink
docs exampleDue to the way the EUI routing system handles hashed URLs, the
EuiSkipLink
component does not currently work on the EUI docs site (i.e. it treats an anchor href likehref="a11ySkipToMain"
as a new URL).Once the upcoming infrastructure work towards a static system is in place, we can hook this up properly on the EUI site. In the meantime, I've added a
TODO
regarding an override in the code and a note on the docs example explaining that the link won't work.For that reason, the "Skip to main content" link will only appear when you tab into the Utilities > Accessibility page (i.e. doesn't work globally) and I'm thus using an
EuiPortal
andtabIndex={1}
to effectively fake it for the time being. Ultimately, this will live near the top of the body.Keyboard focus in Firefox
For mac users, the
skip to content
won't display unless you've complete the two items in the first answer of this SO post: https://stackoverflow.com/questions/11704828/how-to-allow-keyboard-focus-of-links-in-firefoxIE11
It does display in IE11, but the tab order feels slightly off. That said, due to the aforementioned issues with making this work in the EUI docs, I'm using an
EuiPortal
so its not really at the top of markup since I'm not able to make this work globally (yet).Checklist